From 673c30de64bf118af7aa51aab219ba7fc9cdc26e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 11 Nov 2014 11:59:31 -0800 Subject: [PATCH] Fix a panic when building with build scripts A package can be required to be built for both the host and target architectures in some cases. For example a crate could be a normal dependency and a build dependency. Cargo specially handles this case with respect to the build script to ensure that the build script is run once per output platform. Cargo also has logic, however, to run the build script only once when the target and host platforms are the same. In this case Cargo previously wasn't filling in the local build script output cache for both the host and target platforms, just the target platform. This commit remedies this situation by ensuring that cargo populates both the host and target locations in the cache. Closes #838 --- src/cargo/ops/cargo_rustc/context.rs | 27 ++++++++------- src/cargo/ops/cargo_rustc/custom_build.rs | 32 +++++++++++++++--- src/cargo/ops/cargo_rustc/mod.rs | 17 ++++++---- tests/test_cargo_compile_custom_build.rs | 40 +++++++++++++++++++++++ 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs index ee7469253..251aa64b9 100644 --- a/src/cargo/ops/cargo_rustc/context.rs +++ b/src/cargo/ops/cargo_rustc/context.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::collections::hash_map::{HashMap, Occupied, Vacant}; use std::str; use std::sync::Arc; @@ -135,8 +134,7 @@ impl<'a, 'b: 'a> Context<'a, 'b> { let targets = pkg.get_targets().iter(); for target in targets.filter(|t| t.get_profile().is_compile()) { - self.build_requirements(pkg, target, PlatformTarget, - &mut HashSet::new()); + self.build_requirements(pkg, target, PlatformTarget); } self.compilation.extra_env.insert("NUM_JOBS".to_string(), @@ -150,30 +148,31 @@ impl<'a, 'b: 'a> Context<'a, 'b> { } fn build_requirements(&mut self, pkg: &'a Package, target: &'a Target, - req: PlatformRequirement, - visiting: &mut HashSet<(&'a PackageId, &'a str)>) { + req: PlatformRequirement) { - let key = (pkg.get_package_id(), target.get_name()); - if !visiting.insert(key) { return } let req = if target.get_profile().is_for_host() {PlatformPlugin} else {req}; - match self.requirements.entry(key) { - Occupied(mut entry) => { *entry.get_mut() = entry.get().combine(req); } + match self.requirements.entry((pkg.get_package_id(), target.get_name())) { + Occupied(mut entry) => match (*entry.get(), req) { + (PlatformPlugin, PlatformPlugin) | + (PlatformPluginAndTarget, PlatformPlugin) | + (PlatformTarget, PlatformTarget) | + (PlatformPluginAndTarget, PlatformTarget) | + (PlatformPluginAndTarget, PlatformPluginAndTarget) => return, + _ => *entry.get_mut() = entry.get().combine(req), + }, Vacant(entry) => { entry.set(req); } }; for &(pkg, dep) in self.dep_targets(pkg, target).iter() { - self.build_requirements(pkg, dep, req, visiting); + self.build_requirements(pkg, dep, req); } match pkg.get_targets().iter().find(|t| t.get_profile().is_custom_build()) { Some(custom_build) => { - self.build_requirements(pkg, custom_build, PlatformPlugin, - visiting); + self.build_requirements(pkg, custom_build, PlatformPlugin); } None => {} } - - visiting.remove(&key); } pub fn get_requirement(&self, pkg: &'a Package, diff --git a/src/cargo/ops/cargo_rustc/custom_build.rs b/src/cargo/ops/cargo_rustc/custom_build.rs index 2edfea8d5..a6e0261d7 100644 --- a/src/cargo/ops/cargo_rustc/custom_build.rs +++ b/src/cargo/ops/cargo_rustc/custom_build.rs @@ -11,6 +11,8 @@ use util::{internal, ChainError, Require}; use super::job::Work; use super::{fingerprint, process, KindTarget, KindHost, Kind, Context}; +use super::{PlatformPlugin, PlatformPluginAndTarget, PlatformTarget}; +use super::PlatformRequirement; use util::Freshness; /// Contains the parsed output of a custom build script. @@ -29,8 +31,14 @@ pub struct BuildState { } /// Prepares a `Work` that executes the target as a custom build script. -pub fn prepare(pkg: &Package, target: &Target, kind: Kind, cx: &mut Context) - -> CargoResult<(Work, Work, Freshness)> { +/// +/// The `req` given is the requirement which this run of the build script will +/// prepare work for. If the requirement is specified as both the target and the +/// host platforms it is assumed that the two are equal and the build script is +/// only run once (not twice). +pub fn prepare(pkg: &Package, target: &Target, req: PlatformRequirement, + cx: &mut Context) -> CargoResult<(Work, Work, Freshness)> { + let kind = match req { PlatformPlugin => KindHost, _ => KindTarget, }; let (script_output, build_output, old_build_output) = { let target = cx.layout(pkg, kind); (cx.layout(pkg, KindHost).build(pkg), @@ -151,7 +159,7 @@ pub fn prepare(pkg: &Package, target: &Target, kind: Kind, cx: &mut Context) human("build script output was not valid utf-8") })); let parsed_output = try!(BuildOutput::parse(output, pkg_name.as_slice())); - build_state.outputs.lock().insert((id, kind), parsed_output); + build_state.insert(id, req, parsed_output); try!(File::create(&build_output.dir_path().join("output")) .write_str(output).map_err(|e| { @@ -185,7 +193,7 @@ pub fn prepare(pkg: &Package, target: &Target, kind: Kind, cx: &mut Context) let contents = try!(f.read_to_string()); let output = try!(BuildOutput::parse(contents.as_slice(), pkg_name.as_slice())); - build_state.outputs.lock().insert((id, kind), output); + build_state.insert(id, req, output); fresh(tx) }; @@ -220,6 +228,22 @@ impl BuildState { } BuildState { outputs: Mutex::new(outputs) } } + + fn insert(&self, id: PackageId, req: PlatformRequirement, + output: BuildOutput) { + let mut outputs = self.outputs.lock(); + match req { + PlatformTarget => { outputs.insert((id, KindTarget), output); } + PlatformPlugin => { outputs.insert((id, KindHost), output); } + + // If this build output was for both the host and target platforms, + // we need to insert it at both places. + PlatformPluginAndTarget => { + outputs.insert((id.clone(), KindHost), output.clone()); + outputs.insert((id, KindTarget), output); + } + } + } } impl BuildOutput { diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs index 89f4217da..5a4a24a23 100644 --- a/src/cargo/ops/cargo_rustc/mod.rs +++ b/src/cargo/ops/cargo_rustc/mod.rs @@ -219,31 +219,34 @@ fn compile<'a, 'b>(targets: &[&'a Target], pkg: &'a Package, // package is needed in both a host and target context, we need to run // it once per context. if !target.get_profile().is_custom_build() { continue } - let mut kinds = Vec::new(); + let mut reqs = Vec::new(); let requirement = targets.iter().find(|t| { !t.get_profile().is_custom_build() && !t.get_profile().is_doc() }).map(|&other_target| { cx.get_requirement(pkg, other_target) }).unwrap_or(PlatformTarget); match requirement { - PlatformTarget => kinds.push(KindTarget), - PlatformPlugin => kinds.push(KindHost), + PlatformTarget => reqs.push(PlatformTarget), + PlatformPlugin => reqs.push(PlatformPlugin), PlatformPluginAndTarget => { - kinds.push(KindTarget); if cx.config.target().is_some() { - kinds.push(KindHost); + reqs.push(PlatformPlugin); + reqs.push(PlatformTarget); + } else { + reqs.push(PlatformPluginAndTarget); } } } let before = run_custom.len(); - for &kind in kinds.iter() { + for &req in reqs.iter() { + let kind = match req { PlatformPlugin => KindHost, _ => KindTarget }; let key = (pkg.get_package_id().clone(), kind); if pkg.get_manifest().get_links().is_some() && cx.build_state.outputs.lock().contains_key(&key) { continue } let (dirty, fresh, freshness) = - try!(custom_build::prepare(pkg, target, kind, cx)); + try!(custom_build::prepare(pkg, target, req, cx)); run_custom.push((job(dirty, fresh), freshness)); } diff --git a/tests/test_cargo_compile_custom_build.rs b/tests/test_cargo_compile_custom_build.rs index e11dfa6f1..8b80f60db 100644 --- a/tests/test_cargo_compile_custom_build.rs +++ b/tests/test_cargo_compile_custom_build.rs @@ -853,3 +853,43 @@ test!(build_script_only { .with_stderr("either a [lib] or [[bin]] section must \ be present")); }) + +test!(shared_dep_with_a_build_script { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.5.0" + authors = [] + build = "build.rs" + + [dependencies.a] + path = "a" + + [build-dependencies.b] + path = "b" + "#) + .file("src/lib.rs", "") + .file("build.rs", "fn main() {}") + .file("a/Cargo.toml", r#" + [package] + name = "a" + version = "0.5.0" + authors = [] + build = "build.rs" + "#) + .file("a/build.rs", "fn main() {}") + .file("a/src/lib.rs", "") + .file("b/Cargo.toml", r#" + [package] + name = "b" + version = "0.5.0" + authors = [] + + [dependencies.a] + path = "../b" + "#) + .file("b/src/lib.rs", ""); + assert_that(p.cargo_process("build"), + execs().with_status(0)); +}) -- 2.30.2